Skip to content

ASoC: core: only convert non DPCM link to DPCM link#2143

Closed
bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
bardliao:fix-dpcm-overwrite
Closed

ASoC: core: only convert non DPCM link to DPCM link#2143
bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
bardliao:fix-dpcm-overwrite

Conversation

@bardliao
Copy link
Collaborator

@bardliao bardliao commented May 28, 2020

We don't need to overwrite the dpcm_playback and dpcm_capture values
if the link is a DCPM link.

Fixes: 218fe9b ("ASoC: soc-core: Set dpcm_playback / dpcm_capture")

This PR will fix the unexpected error such as "CPU DAI iDisp1 for rtd iDisp1 does not support capture" when #2070 is applied.

dai_link->no_pcm = 1;
dai_link->dpcm_playback = 1;
dai_link->dpcm_capture = 1;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao why we get BE here ? this function only deal with FE in topology.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang If soc_check_tplg_fes find a matched machine (like sof_sdw), it will overwrite all preset links. See for_each_card_prelinks(card, i, dai_link)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so soc_check_tplg_fes doesn't only check fes. why force dpcm_playback & dpcm_capture to 1 ? I don't get it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang @bardliao i think I'm at fault for this

commit 218fe9b7ec7f32c10a07539365488d80af7b0084
Author: Daniel Baluta <daniel.baluta@nxp.com>
Date:   Wed Dec 4 17:13:33 2019 +0200

    ASoC: soc-core: Set dpcm_playback / dpcm_capture
    
    When converting a normal link to a DPCM link we need
    to set dpcm_playback / dpcm_capture otherwise playback/capture
    streams will not be created resulting in errors like this:
    
    [   36.039111]  sai1-wm8960-hifi: ASoC: no backend playback stream
    
    Fixes: a655de808cbde ("ASoC: core: Allow topology to override machine driver FE DAI link config")

I think the proper fix would be to check if the original link has playback OR capture capability and update only that capability instead of updating both playback and capture.

@bardliao bardliao force-pushed the fix-dpcm-overwrite branch from dc9fcd2 to 150c1f3 Compare May 28, 2020 08:32
@bardliao bardliao changed the title [RFC] ASoC: core: only convert non BE into BE ASoC: core: only convert non DPCM link to DPCM link May 28, 2020
if (!strcmp(component->driver->ignore_machine,
card->dev->driver->name))
goto match;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unrelated. But looking at the code I'd rather remove the goto by using

	if (strcmp(component->driver->ignore_machine, card->dev->driver->name) &&
	    strcmp(component->driver->ignore_machine, dev_name(card->dev)))
		continue;

in a separate PR of course

@@ -1656,9 +1657,14 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
dai_link->platforms->name = component->name;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not for this PR, but in line 1649/1650 above card->dai_link[i].name should be dai_link->name

dai_link->dpcm_capture = 1;
dai_link->no_pcm = 1;

/* convert a normal link to a DPCM link */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this moment in code we can reach if dai_link is a normal link (no_pcm = 0, dynamic=0) or a BE link (no_pcm=1).

If the link is already BE, we should do nothing.
if the link is normal link, we should convert it to BE.
-> no_pcm = 1
-> now here is the challenge as dai_link doesn't have a .capture or .playback field.

It has:

-> playback_only=1
   -> .dpcm_playback = 1
   -> dpcm_capture = 0
-> capture_only =1
   -> .dpcm_playback = 0
   -> .dpcm_capture =1
-> BUT what does capture_only = 0 and playback_only = 0 means, logically it could be either bot playback/capture supported or none.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUT what does capture_only = 0 and playback_only = 0 means, logically it could be either bot playback/capture supported or none.

I thought about the capture_only and playback_only flag, too. But it seems it is set by a few machine driver only. I am not sure if we can relay on the capture_only and playback_only flag or not. The other question is that is there any link supports none of playback and capture?

Copy link
Collaborator

@dbaluta dbaluta May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao If I would have to guess I would say that not-supporting playback/capture makes no sense. So, I think we can infer that by default if capture_only is not set and playback_only is not set then both playback/capture are supported.
I have sent an email to alsa mailing list to clarify this situation:

https://mailman.alsa-project.org/pipermail/alsa-devel/2020-May/168352.html

I think the best way to fix this is as follows:

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 8321e75ff244..464004648bd0 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1722,9 +1722,18 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
                        dai_link->platforms->name = component->name;
 
                        /* convert non BE into BE */
-                       dai_link->no_pcm = 1;
-                       dai_link->dpcm_playback = 1;
-                       dai_link->dpcm_capture = 1;
+                       if (!dai_link->no_pcm) {
+
+                               dai_link->no_pcm = 1;
+
+                               dai_link->dpcm_playback = 1;
+                               dai_link->dpcm_capture = 1;
+
+                               if (dai_link->playback_only)
+                                       dai_link->dpcm_capture = 0;
+                               if (dai_link->capture_only)
+                                       dai_link->dpcm_playback = 0;
+                       }

Copy link
Member

@plbossart plbossart May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaluta I asked the question on alsa-devel on why we have separate flags for dpcm and non-dpcm solutions. I think it's a complete mess. We should have .pcm_playback and .pcm_capture flags only, you can infer playback_only and capture_only when only one of the two is set.

We don't need to overwrite the dpcm_playback and dpcm_capture values
if the link is a DCPM link.

Fixes: 218fe9b ("ASoC: soc-core: Set dpcm_playback / dpcm_capture")
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
@plbossart
Copy link
Member

@bardliao please check PR #2070

@bardliao bardliao closed this Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants